Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove Amino #265

Merged
merged 7 commits into from
Jun 30, 2020
Merged

remove Amino #265

merged 7 commits into from
Jun 30, 2020

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Jun 16, 2020

Removes Amino and adds varint utility-functions instead, keeping the same serialized format. Partial fix for #242.

The last vestige of Amino is in the proof handling, namely AbsenceOp and ValueOp, which are structs encoded/decoded with Amino.

@AdityaSripal
Copy link
Member

Amino can be removed from the proofs. This will affect the IavlSpec in ICS-23 since the prefix would change. Think it's worth it to purge iAVL of amino though.

Think #276 should be merged after this PR, and IavlSpec needs to be changed in https://github.com/confio/ics23

Please confirm @ethanfrey

@ethanfrey
Copy link
Contributor

As to ics23 proofs, we allow a spec to length-prefix data many ways. Ideally the output of binary.Varint is compatible with one of the supported methods. If so, we just update the spec along with the new proofs. If this is different, please make a PR to add this to ics23 (we need it in proto and 3 languages) or use a supported format, or wait til I have bandwidth for this.

@ethanfrey
Copy link
Contributor

It looks like binary.Varint is protobuf compatible:

The varint functions encode and decode single integer values using a variable-length encoding; smaller values require fewer bytes. For a specification, see https://developers.google.com/protocol-buffers/docs/encoding. 

So this shouldn't even be an issue for IavlSpec (but please test)

@ethanfrey
Copy link
Contributor

I would also say that once #276 is merged and deeply integrated (in a separate PR?) to access the store rather than translating exisiting proofs, you may wish to remove the custom amino-style proof format. How are RangeProofs serialized? Maybe not in this module, but in the sdk they are only serialized using amino before hitting the wire on sdk queries.

@erikgrinaker
Copy link
Contributor Author

erikgrinaker commented Jun 28, 2020

Following discussions in a call last week with @AdityaSripal and @alexanderbez, we'll keep the current proofs along with the new proofs, but try to migrate them to Protobuf if we are able to keep byte-compatibility.

I'll do that in a separate PR, so this PR is good to review and merge.

@erikgrinaker erikgrinaker marked this pull request as ready for review June 28, 2020 14:23
@erikgrinaker erikgrinaker added the S:automerge Automatic merge and/or update Pull requests label Jun 30, 2020
@mergify mergify bot merged commit 5d5829f into master Jun 30, 2020
@mergify mergify bot deleted the erik/remove-amino branch June 30, 2020 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:automerge Automatic merge and/or update Pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants